-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NSIB support for nRF54LX series #16765
Conversation
Test specificationCI/Jenkins/NRF
CI/Jenkins/integration
All integration tests: null Detailed information of selected test modules Note: This message is automatically posted and updated by the CI |
You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds. Note: This comment is automatically posted by the Documentation Publishing GitHub Action. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addititionally, please add the new platform(s) to tests/subsys/bootloader/bl_storage/testcase.yaml and update tests/subsys/bootloader/bl_storage/src/main.c to use the new typedefs.
if (IS_ENABLED(CONFIG_FPROTECT)) { | ||
err = fprotect_area(PM_B0_ADDRESS, PM_B0_SIZE); | ||
} else { | ||
printk("Fprotect disabled. No protection applied.\n\r"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When is this relevant? Is it even a valid configuration, or should if fail more loudly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nRF54l15 won't use runtime setup for memory overwrite protection.
scripts/bootloader/provision.py
Outdated
@@ -132,6 +131,8 @@ def parse_args(): | |||
help="The MCUBOOT bootloader is used without the NSIB bootloader. Only the provision address, the MCUBOOT counters and the MCUBOOT counters slots arguments will be used.") | |||
parser.add_argument('--mcuboot-counters-slots', required=False, type=int, default=0, | |||
help='Number of monotonic counter slots for every MCUBOOT counter.') | |||
parser.add_argument('--lcs-state-size', required=False, type=lambda x: int(x, 0), default=0x8, | |||
help='Number of monotonic counter slots for every MCUBOOT counter.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
helptext needs updating
scripts/bootloader/provision.py
Outdated
@@ -153,8 +154,13 @@ def get_hashes(public_key_files, verify_hashes): | |||
|
|||
|
|||
def main(): | |||
global lcs_state_size |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be refactored to not be a global variable. Making it a parameter to generate_mcuboot_only_provision_hex_file
should be enough.
include/bl_storage.h
Outdated
uint16_t secure; | ||
lcs_data_t provisioning; | ||
lcs_data_t secure; | ||
#ifdef CONFIG_NRFX_NVMC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if it's a good idea to remove the padding. It's not needed for padding purposes, but will be needed if another state is added. Maybe @Vge0rge can chime in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe worth to add reserved for future usage field for RRAM based device as well then.
Just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it's better to have a reserve word there in case we want to support another state later.
include/bl_storage.h
Outdated
@@ -238,6 +290,87 @@ NRFX_STATIC_INLINE void read_implementation_id_from_otp(uint8_t *buf) | |||
* This is a temporary solution until TF-M has access to NSIB functions. | |||
*/ | |||
|
|||
#ifdef CONFIG_NRFX_RRAMC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the added functions are basically internal functions, so I think it's less than ideal to have them in the header file. I guess there's precedent for it in this file though, so maybe not in scope to fix here, but maybe just give it a second thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
checked that requests from original PR were fulfilled.
43e2278
to
a619902
Compare
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 74cce6bc8c325137702fa6207d9aef3f0eb3ef94 more detailssdk-nrf:
Github labels
List of changed files detected by CI (25)
Outputs:ToolchainVersion: b44b7a08c9 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
ea131cb
to
f2c25e9
Compare
@michalek-no Can you rebase? |
f2c25e9
to
3ba346d
Compare
rebase |
3ba346d
to
c168216
Compare
rebase |
a098069
to
b043360
Compare
b043360
to
3d66ec5
Compare
Since the vector table of nRF54LX series is larger than before using it is nice to have an offset between `0x400` and `0x800` to store `fw_info` in to ensure you can tightly pack your application together with firmware info. One downside of this is that this new offset is not supported in any older version of NSIB. Signed-off-by: Mateusz Michalek <[email protected]>
Added OTP region in partition manager for `nRF54L15X_ENGA` devices and modified the secure boot storage partition to use OTP for NRF54L15. Ref. NCSDK-25306 Signed-off-by: Mateusz Michalek <[email protected]>
Changed the implementation of bl_storage to be compatible with RRAMC and FLASH/NVMC. Due to the limitations in the RRAM controller OTP can only be written to by words not half-words as it can be in NVMC. This means we need to change the way we handle certain cases in `bl_storage.c`, `bl_validation.c` and `fw_info.c`. This commit moves some of the abstractions into two separate files `bl_storage_nvmc.c` and `bl_storage_rramc.c`. However there are multiple other changes needed which are done by using `#ifdef`'s. Ref. NCSDK-25306 Signed-off-by: Mateusz Michalek <[email protected]>
Changed the provisioning script to account for larger Life Cycle State structures. Since we can only do word writes in nRF54LX life cycle states needs to be stored in words. This increased the LCS struct by 1 word requiring us to change the way the provisioning script works. This change adds the LCS size as a value to the CMake file which uses the SOC to decide which size the LCS struct is which is then passed to the python script through the `--lcs-state-size` argument. Ref. NCSDK-25306 Signed-off-by: Mateusz Michalek <[email protected]> Signed-off-by: Sigurd Hellesvik <[email protected]>
Instead of just failing on compilation this commit changes fprotect requirements so that it prints out a warning and error in log. Signed-off-by: Mateusz Michalek <[email protected]>
Selects NRFX_RRAMC symbol required by fprotect RRAM backend on 54L15 platform. Signed-off-by: Mateusz Michalek <[email protected]>
Adds bl_storage tests. Signed-off-by: Mateusz Michalek <[email protected]>
Add nrf54l15dk to boot_chains configurations. Signed-off-by: Grzegorz Chwierut <[email protected]>
adds another UART's uninit call. Signed-off-by: Mateusz Michalek <[email protected]>
NRFX_NVMC is being selected conditionally as it is not the one and only memory backend anymore. Signed-off-by: Mateusz Michalek <[email protected]>
3d66ec5
to
74cce6b
Compare
release notes tba to the #18192 PR |
int err = fprotect_area(PM_B0_ADDRESS, PM_B0_SIZE); | ||
int err = 0; | ||
|
||
if (IS_ENABLED(CONFIG_FPROTECT)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this a ifdef?
int err = fprotect_area(PM_PROVISION_ADDRESS, PM_PROVISION_SIZE); | ||
int err = 0; | ||
|
||
if (IS_ENABLED(CONFIG_FPROTECT)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why isn't this an ifdef?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I also tested dual bootloaders and MCUboot upgrade with this. That worked as expected.
@frkv We need to merge that ASAP. #Ifdefry can be improved later. |
agreed with @nvlsianpu to merge this PR to main only. |
duplicate of #16079 due to CI failing over contributing author.